Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add target parameter, make it major by default #47

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

fahric
Copy link
Contributor

@fahric fahric commented Jun 30, 2021

Closes #44

Checklist

@simoneb simoneb changed the title wip: add tests,add new target parameter,make it major by default #46 add tests,add new target parameter,make it major by default #46 Jun 30, 2021
Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments

README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/lib/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
src/lib/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
src/lib/getTargetInput.js Outdated Show resolved Hide resolved
src/lib/getTargetInput.js Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
@simoneb
Copy link
Collaborator

simoneb commented Jun 30, 2021

Although it's not enforced in this repo, let's try to stick to conventional commits when committing to this repo. Also stick to shorter commit messages as per git's guidelines (below 50 chars on the first line of the commit). Please rebase and squash commits to make it a single one which adheres to the above.

@simoneb
Copy link
Collaborator

simoneb commented Jun 30, 2021

Also please modify the workflow file to run linting and testing and make the automerge job depend on it. Inspire from https://github.com/nearform/get-jwks/blob/main/.github/workflows/ci.yml (no need for caching node_modules though). Note the syntax of the npm lint script looks wrong, I think it should be eslint ..

@simoneb
Copy link
Collaborator

simoneb commented Jun 30, 2021

the ci workflow should also run on push, not just pull request. can you please make that change too?

@fahric fahric requested a review from simoneb July 1, 2021 08:01
Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see inline comments

.github/workflows/ci.yml Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
src/checkTargetMatchToPR.js Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
@fahric fahric requested a review from simoneb July 1, 2021 14:58
src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
@fahric fahric requested a review from simoneb July 2, 2021 08:45
@simoneb
Copy link
Collaborator

simoneb commented Jul 2, 2021

please rebase on main branch and resolve conflicts

src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
src/checkTargetMatchToPR.js Outdated Show resolved Hide resolved
test/checkTargetMatchToPR.test.js Outdated Show resolved Hide resolved
test/getTargetInput.test.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@fahric fahric requested a review from simoneb July 2, 2021 09:16
.taprc Outdated Show resolved Hide resolved
@fahric fahric marked this pull request as ready for review July 2, 2021 10:16
@fahric fahric requested a review from climba03003 July 2, 2021 12:22
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@fahric fahric requested a review from simoneb July 2, 2021 13:14
@fahric fahric requested a review from climba03003 July 2, 2021 14:03
@fahric fahric changed the title add tests,add new target parameter,make it major by default #46 feat: add target parameter,make it major by default Jul 2, 2021
@fahric fahric changed the title feat: add target parameter,make it major by default feat: add target parameter, make it major by default Jul 2, 2021
@simoneb simoneb merged commit 3f5ef07 into fastify:main Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-Merge Flag to Limit to Minor/Patch Updates
3 participants